Fix: reject '@' in HTTP header field names per RFC 9110 #12838
Conversation
RFC 9110 defines HTTP header field names as tokens, which must not contain '@'. Currently '@' is accepted because of MIME separator handling. This patch ensures ParseRules::is_http_field_name rejects '@', aligning behavior with RFC 9110. This prevents invalid header names from being accepted.
There was a problem hiding this comment.
Pull request overview
This PR fixes HTTP header field name validation to comply with RFC 9110 by rejecting the '@' character. The current implementation incorrectly accepts '@' due to MIME separator handling logic.
Changes:
- Removed the exception that allowed '@' in HTTP header field names
- Updated the validation logic to properly reject all MIME separators including '@'
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| { | ||
| #ifndef COMPILE_PARSE_RULES | ||
| return (parseRulesCType[static_cast<unsigned char>(c)] & is_token_BIT); | ||
| return (parseRulesCType[(unsigned char)c] & is_token_BIT); |
There was a problem hiding this comment.
Changed from static_cast<unsigned char> to C-style cast (unsigned char). C++ best practice is to use static_cast for type safety and clarity. This change appears unrelated to the PR's stated purpose of fixing '@' validation.
| return (parseRulesCType[(unsigned char)c] & is_token_BIT); | |
| return (parseRulesCType[static_cast<unsigned char>(c)] & is_token_BIT); |
| { | ||
| #ifndef COMPILE_PARSE_RULES | ||
| return (parseRulesCType[static_cast<unsigned char>(c)] & is_token_BIT); | ||
| return (parseRulesCType[(unsigned char)c] & is_token_BIT); |
|
Looks like some of the catch tests failed: |
bneradt
left a comment
bneradt
left a comment
There was a problem hiding this comment.
Thanks for looking into RFC compliance. However, this change breaks the unit tests because ATS intentionally allows @ in header field names for internal headers.
Why the test fails
The test at src/proxy/hdrs/unit_tests/test_Hdrs.cc:583 expects @ to be valid:
{"delimiter_@: 0\r\n", ParseResult::CONT }, // Not allowed by the spec, but we use it as internal header indicatorThe comment explicitly acknowledges this is a deliberate deviation from the spec.
Internal header usage
ATS uses @ as a prefix convention for internal headers:
@Ats-Internalis defined as a well-known header token insrc/proxy/hdrs/HdrToken.cc:90MIME_FIELD_ATS_INTERNALreferences this insrc/proxy/hdrs/MIME.cc:716- The aws_auth_v4 plugin explicitly skips headers starting with
@because they're internal (plugins/origin_server_auth/aws_auth_v4.cc:434-435):/* Skip internal headers (starting with '@'*/ if ('@' == name[0] /* exclude internal headers */) {
- The ESI plugin tests use
@-prefixed headers for internal testing
Recommendation
The original exception (is_mime_sep(c) && c != '@') was intentional. If strict RFC 9110 compliance is desired, this would require a broader refactoring effort to:
- Replace
@Ats-Internalwith an alternative internal header mechanism - Update all plugins that rely on the
@prefix convention - Update the unit test expectations
As it stands, simply removing the @ exception will break internal functionality.
Is there a specific issue you are trying to address with this patch? Maybe we can address it in a different way.
|
I'm going to go ahead and close this given my above comment explaining why we need @ header parsing. Feel free to reply if you have other thoughts. |
3 participants
RFC 9110 defines HTTP header field names as tokens, which must not contain '@'.
Currently '@' is accepted because of MIME separator handling.
This patch ensures ParseRules::is_http_field_name rejects '@', aligning behavior with RFC 9110.
This prevents invalid header names from being accepted.